i18n: handle string placeholder collisions #14432
Conversation
brendankenny
requested review from
connorjclark
and removed request for
a team
| @@ -211,6 +211,23 @@ CTC is a name that is distinct and identifies this as the Chrome translation for | |||
| } | |||
| ``` | |||
|
|
|||
| ### Collisions | |||
There was a problem hiding this comment.
history has shown we pretty reliably forget this information over time, so please give any feedback on ambiguity so we can clear it up now :)
| * @param {(element: T) => U} callback | ||
| * @return {Map<U, Array<T>>} | ||
| */ | ||
| function arrayGroupToMap(elements, callback) { |
There was a problem hiding this comment.
Had to do this twice (nested) and was getting kind of gross. It's a nice API (for Node 20 or whatever)
core/scripts/i18n/collect-strings.js
Outdated
| * Check that the known collisions match our known list. | ||
| * @param {Array<CtcMessage>} fixedCollisions | ||
| */ | ||
| function checkKnownCollisions(fixedCollisions) { |
There was a problem hiding this comment.
split this into a different function because otherwise the only way to test resolveMessageCollisions would be against all the strings collected from lighthouse (since all the known collisions are asserted to be there). If this file gets much more testing, it probably just needs to not interleave reading/writing disk and the parsing/string creation code
brendankenny
force-pushed
the
string-collisions
branch
from
d53ba56
to
c364fa3
Compare
brendankenny
force-pushed
the
string-collisions
branch
from
c364fa3
to
2c3b22c
Compare
| /** Description of a Lighthouse audit that tells the user *why* they should have accessible names for HTML elements. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */ | ||
| description: 'When an element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn how to name `tooltip` elements](https://dequeuniversity.com/rules/axe/4.4/aria-tooltip-name).', | ||
| /** Description of a Lighthouse audit that tells the user *why* they should have accessible names for HTML 'tooltip' elements. This is displayed after a user expands the section to see more. No character length limits. 'Learn how...' becomes link text to additional documentation. */ | ||
| description: 'When a tooltip element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn how to name `tooltip` elements](https://dequeuniversity.com/rules/axe/4.4/aria-tooltip-name).', |
There was a problem hiding this comment.
should we mark tooltip/meter with backticks?
There was a problem hiding this comment.
I took that from the axe docs (e.g. "Aria tooltip elements must have discernible text..."), though to be fair, their docs language selector has only one other language and that particular string stays in english :)
I kind of justified it in my head as it's an aria role, but also a description of the type of element, so it makes some sense to include a localized name. tooltip (and meter for aria-meter-name) are backticked later in the string 🤷
edit: I could make the second reference more explicitly the markup version, Learn how to name `role="tooltip" ` elements
| return JSON.parse(JSON.stringify(input)); | ||
| } | ||
|
|
||
| /** @return {Record<'first'|'second'|'third', CtcWithPlaceholders>} */ |
There was a problem hiding this comment.
I wonder if we'll be able to use satisfies w/ jsdoc... https://devblogs.microsoft.com/typescript/announcing-typescript-4-9-beta/#the-satisfies-operator
There was a problem hiding this comment.
That would be really nice. I can't make it trigger and the PR doesn't include any jsdoc tests, so I assume it's a different enough parsing flow that it doesn't get picked up in any jsdoc equivalent.
There was a problem hiding this comment.
you motivated me: microsoft/TypeScript#51086 :)
There was a collision error when importing the latest strings to TC due to a difference in placeholders in
core/audits/accessibility/aria-meter-name.js | descriptionandcore/audits/accessibility/aria-tooltip-name.js | description.Chain of events:
aria-meter-nameandaria-tooltip-namehad the exact same placeholders, so the error wasn't apparent. report: expand on "learn more" links #14091 changed the Learn More links so they had different placeholders, triggering the bug.With this PR, to count as "allowed" collisions,
placeholdersmust also be exact matches, orcollect-stringswill suggest changing the stringdescriptionto be a fixable collision.I also stepped through a bunch of the TC code to try to figure out collisions once and for all(tm) and documented it in the i18n readme for future reference.